-
Notifications
You must be signed in to change notification settings - Fork 15.4k
[clang-tidy] Extend bugprone-use-after-move to allow custom invalidation functions
#170346
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[clang-tidy] Extend bugprone-use-after-move to allow custom invalidation functions
#170346
Conversation
|
@llvm/pr-subscribers-clang-tools-extra @llvm/pr-subscribers-clang-tidy Author: None (higher-performance) ChangesFull diff: https://github.com/llvm/llvm-project/pull/170346.diff 3 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
index efb5ec64689cf..404fc4de8ef3c 100644
--- a/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
@@ -19,6 +19,7 @@
#include "../utils/ExprSequence.h"
#include "../utils/Matchers.h"
+#include "../utils/OptionsUtils.h"
#include <optional>
using namespace clang::ast_matchers;
@@ -48,7 +49,8 @@ struct UseAfterMove {
/// various internal helper functions).
class UseAfterMoveFinder {
public:
- UseAfterMoveFinder(ASTContext *TheContext);
+ UseAfterMoveFinder(ASTContext *TheContext,
+ llvm::ArrayRef<StringRef> InvalidationFunctions);
// Within the given code block, finds the first use of 'MovedVariable' that
// occurs after 'MovingCall' (the expression that performs the move). If a
@@ -71,11 +73,17 @@ class UseAfterMoveFinder {
llvm::SmallPtrSetImpl<const DeclRefExpr *> *DeclRefs);
ASTContext *Context;
+ llvm::ArrayRef<StringRef> InvalidationFunctions;
std::unique_ptr<ExprSequence> Sequence;
std::unique_ptr<StmtToBlockMap> BlockMap;
llvm::SmallPtrSet<const CFGBlock *, 8> Visited;
};
+static auto getNameMatcher(llvm::ArrayRef<StringRef> InvalidationFunctions) {
+ return anyOf(hasAnyName("::std::move", "::std::forward"),
+ matchers::matchesAnyListedName(InvalidationFunctions));
+}
+
} // namespace
// Matches nodes that are
@@ -92,8 +100,9 @@ static StatementMatcher inDecltypeOrTemplateArg() {
hasAncestor(expr(hasUnevaluatedContext())));
}
-UseAfterMoveFinder::UseAfterMoveFinder(ASTContext *TheContext)
- : Context(TheContext) {}
+UseAfterMoveFinder::UseAfterMoveFinder(
+ ASTContext *TheContext, llvm::ArrayRef<StringRef> InvalidationFunctions)
+ : Context(TheContext), InvalidationFunctions(InvalidationFunctions) {}
std::optional<UseAfterMove>
UseAfterMoveFinder::find(Stmt *CodeBlock, const Expr *MovingCall,
@@ -359,7 +368,7 @@ void UseAfterMoveFinder::getReinits(
unless(parmVarDecl(hasType(
references(qualType(isConstQualified())))))),
unless(callee(functionDecl(
- hasAnyName("::std::move", "::std::forward")))))))
+ getNameMatcher(InvalidationFunctions)))))))
.bind("reinit");
Stmts->clear();
@@ -389,8 +398,9 @@ void UseAfterMoveFinder::getReinits(
}
enum class MoveType {
- Move, // std::move
- Forward, // std::forward
+ Forward, // std::forward
+ Move, // std::move
+ Invalidation, // other
};
static MoveType determineMoveType(const FunctionDecl *FuncDecl) {
@@ -399,7 +409,7 @@ static MoveType determineMoveType(const FunctionDecl *FuncDecl) {
if (FuncDecl->getName() == "forward")
return MoveType::Forward;
- llvm_unreachable("Invalid move type");
+ return MoveType::Invalidation;
}
static void emitDiagnostic(const Expr *MovingCall, const DeclRefExpr *MoveArg,
@@ -408,29 +418,40 @@ static void emitDiagnostic(const Expr *MovingCall, const DeclRefExpr *MoveArg,
const SourceLocation UseLoc = Use.DeclRef->getExprLoc();
const SourceLocation MoveLoc = MovingCall->getExprLoc();
- const bool IsMove = (Type == MoveType::Move);
+ const int Kind = static_cast<int>(Type);
- Check->diag(UseLoc, "'%0' used after it was %select{forwarded|moved}1")
- << MoveArg->getDecl()->getName() << IsMove;
- Check->diag(MoveLoc, "%select{forward|move}0 occurred here",
+ Check->diag(UseLoc,
+ "'%0' used after it was %select{forwarded|moved|invalidated}1")
+ << MoveArg->getDecl()->getName() << Kind;
+ Check->diag(MoveLoc, "%select{forward|move|invalidation}0 occurred here",
DiagnosticIDs::Note)
- << IsMove;
+ << Kind;
if (Use.EvaluationOrderUndefined) {
Check->diag(
UseLoc,
- "the use and %select{forward|move}0 are unsequenced, i.e. "
+ "the use and %select{forward|move|invalidation}0 are unsequenced, i.e. "
"there is no guarantee about the order in which they are evaluated",
DiagnosticIDs::Note)
- << IsMove;
+ << Kind;
} else if (Use.UseHappensInLaterLoopIteration) {
Check->diag(UseLoc,
"the use happens in a later loop iteration than the "
- "%select{forward|move}0",
+ "%select{forward|move|invalidation}0",
DiagnosticIDs::Note)
- << IsMove;
+ << Kind;
}
}
+UseAfterMoveCheck::UseAfterMoveCheck(StringRef Name, ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context),
+ InvalidationFunctions(utils::options::parseStringList(
+ Options.get("InvalidationFunctions", ""))) {}
+
+void UseAfterMoveCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
+ Options.store(Opts, "InvalidationFunctions",
+ utils::options::serializeStringList(InvalidationFunctions));
+}
+
void UseAfterMoveCheck::registerMatchers(MatchFinder *Finder) {
// try_emplace is a common maybe-moving function that returns a
// bool to tell callers whether it moved. Ignore std::move inside
@@ -438,11 +459,14 @@ void UseAfterMoveCheck::registerMatchers(MatchFinder *Finder) {
// the bool.
auto TryEmplaceMatcher =
cxxMemberCallExpr(callee(cxxMethodDecl(hasName("try_emplace"))));
+ auto Arg = declRefExpr().bind("arg");
+ auto IsMemberCallee = callee(functionDecl(unless(isStaticStorageClass())));
auto CallMoveMatcher =
- callExpr(argumentCountIs(1),
- callee(functionDecl(hasAnyName("::std::move", "::std::forward"))
+ callExpr(callee(functionDecl(getNameMatcher(InvalidationFunctions))
.bind("move-decl")),
- hasArgument(0, declRefExpr().bind("arg")),
+ anyOf(cxxMemberCallExpr(IsMemberCallee, on(Arg)),
+ callExpr(unless(cxxMemberCallExpr(IsMemberCallee)),
+ hasArgument(0, Arg))),
unless(inDecltypeOrTemplateArg()),
unless(hasParent(TryEmplaceMatcher)), expr().bind("call-move"),
anyOf(hasAncestor(compoundStmt(
@@ -521,7 +545,7 @@ void UseAfterMoveCheck::check(const MatchFinder::MatchResult &Result) {
}
for (Stmt *CodeBlock : CodeBlocks) {
- UseAfterMoveFinder Finder(Result.Context);
+ UseAfterMoveFinder Finder(Result.Context, InvalidationFunctions);
if (auto Use = Finder.find(CodeBlock, MovingCall, Arg))
emitDiagnostic(MovingCall, Arg, *Use, this, Result.Context,
determineMoveType(MoveDecl));
diff --git a/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.h b/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.h
index d38b29e09fa8b..1bbf5c00785ff 100644
--- a/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.h
+++ b/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.h
@@ -20,13 +20,16 @@ namespace clang::tidy::bugprone {
/// https://clang.llvm.org/extra/clang-tidy/checks/bugprone/use-after-move.html
class UseAfterMoveCheck : public ClangTidyCheck {
public:
- UseAfterMoveCheck(StringRef Name, ClangTidyContext *Context)
- : ClangTidyCheck(Name, Context) {}
+ UseAfterMoveCheck(StringRef Name, ClangTidyContext *Context);
+ void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
return LangOpts.CPlusPlus11;
}
void registerMatchers(ast_matchers::MatchFinder *Finder) override;
void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+private:
+ std::vector<StringRef> InvalidationFunctions;
};
} // namespace clang::tidy::bugprone
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp
index 87dfec4f68061..57920f3bd7657 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp
@@ -1,5 +1,5 @@
-// RUN: %check_clang_tidy -std=c++11 -check-suffixes=,CXX11 %s bugprone-use-after-move %t -- -- -fno-delayed-template-parsing
-// RUN: %check_clang_tidy -std=c++17-or-later %s bugprone-use-after-move %t -- -- -fno-delayed-template-parsing
+// RUN: %check_clang_tidy -std=c++11 -check-suffixes=,CXX11 %s bugprone-use-after-move %t -- -config="{CheckOptions: {bugprone-use-after-move.InvalidationFunctions: 'Database::StaticCloseConnection;Database::CloseConnection;FriendCloseConnection'}}" -- -fno-delayed-template-parsing
+// RUN: %check_clang_tidy -std=c++17-or-later %s bugprone-use-after-move %t -- -config="{CheckOptions: {bugprone-use-after-move.InvalidationFunctions: 'Database::StaticCloseConnection;Database::CloseConnection;FriendCloseConnection'}}" -- -fno-delayed-template-parsing
typedef decltype(nullptr) nullptr_t;
@@ -1645,3 +1645,41 @@ void create() {
}
} // namespace issue82023
+
+namespace custom_invalidation
+{
+
+struct Database {
+ void CloseConnection();
+ static void StaticCloseConnection(Database&);
+ friend void FriendCloseConnection(Database&);
+ void Query();
+};
+
+void Run() {
+ Database db1;
+ db1.CloseConnection();
+ db1.Query();
+ // CHECK-NOTES: [[@LINE-1]]:3: warning: 'db1' used after it was invalidated
+ // CHECK-NOTES: [[@LINE-3]]:7: note: invalidation occurred here
+
+ Database db2;
+ Database::StaticCloseConnection(db2);
+ db2.Query();
+ // CHECK-NOTES: [[@LINE-1]]:3: warning: 'db2' used after it was invalidated
+ // CHECK-NOTES: [[@LINE-3]]:3: note: invalidation occurred here
+
+ Database db3;
+ Database().StaticCloseConnection(db3);
+ db3.Query();
+ // CHECK-NOTES: [[@LINE-1]]:3: warning: 'db3' used after it was invalidated
+ // CHECK-NOTES: [[@LINE-3]]:3: note: invalidation occurred here
+
+ Database db4;
+ FriendCloseConnection(db4);
+ db4.Query();
+ // CHECK-NOTES: [[@LINE-1]]:3: warning: 'db4' used after it was invalidated
+ // CHECK-NOTES: [[@LINE-3]]:3: note: invalidation occurred here
+}
+
+} // namespace custom_invalidation
|
|
✅ With the latest revision this PR passed the C/C++ code linter. |
8fbbf33 to
0855ab7
Compare
vbvictor
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add release notes and documentation for new option
0855ab7 to
b3dc437
Compare
clang-tools-extra/docs/clang-tidy/checks/bugprone/use-after-move.rst
Outdated
Show resolved
Hide resolved
clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp
Outdated
Show resolved
Hide resolved
63cd086 to
8b65f32
Compare
66f7659 to
1ecfecc
Compare
1ecfecc to
3fca92f
Compare
bugprone-use-after-move to allow custom invalidation functions
zeyi2
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But would be nice to wait for other reviewers :)
3fca92f to
b3ace44
Compare
| anyOf(cxxMemberCallExpr(IsMemberCallee, on(Arg)), | ||
| callExpr(unless(cxxMemberCallExpr(IsMemberCallee)), | ||
| hasArgument(0, Arg))), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the invalidated object o to detect, you are matching
o.destroy();
destroy(o);
Class::destroy(o);but not
shredder.destroy(o);But I see why it's not supported: is this or the first arg the things that is getting invalidated.
One solution would be to have a separate option list for either case, but that might confuse people. I think the cases you do match are the ones that are more likely to happen, so it might be fine. Just wanted to point this out if others think we should support the one that is currently not matched.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, I deliberately didn't try to generalize in this PR, since as you mentioned I expect that to be much less common, and I don't have a use case for it yet. There are other cases to consider too (like emplace), but if people want it later we can always extend it further.
clang-tools-extra/docs/clang-tidy/checks/bugprone/use-after-move.rst
Outdated
Show resolved
Hide resolved
|
It seems to me this is good enough to merge? If you see any issues as blockers, please let me know. Otherwise I'll probably try to merge later today. |
vbvictor
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
clang-tools-extra/docs/clang-tidy/checks/bugprone/use-after-move.rst
Outdated
Show resolved
Hide resolved
b3ace44 to
47bab32
Compare
47bab32 to
6f3e917
Compare
No description provided.